This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

• PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

sequenceDiagram
    participant File as External MCP File
    participant Discover as discover.ts (readJsonSafe)
    participant ConfigPaths as ConfigPaths.parseText
    participant Index as mcp/index.ts (resolveEnvVars)
    participant Transport as StdioClientTransport

    File->>Discover: provide raw config text
    Discover->>ConfigPaths: parseText(text, filePath, "empty")
    alt parseText succeeds
        ConfigPaths-->>Discover: parsed + interpolated MCP definition
        Discover-->>Index: return MCP definition
        Index->>Index: resolveEnvVars(environment)
        Index-->>Transport: pass resolved environment
    else parseText fails
        ConfigPaths-->>Discover: throw/error
        Discover->>Discover: fallback to parseJsonc(text, errors, {allowTrailingComma:true})
        Discover-->>Index: return parsed MCP definition (uninterpolated if errors)
    end

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Testing: MCP env-var interpolation fix

The problem: When users configure MCP servers with environment variable references like "GITLAB_TOKEN": "${GITLAB_TOKEN}", the MCP server receives the literal string "${GITLAB_TOKEN}" instead of the actual token value, causing auth failures (HTTP 401).

Why it happens: PR #655 (v0.5.19) added ${VAR} interpolation to the config parser, but two code paths bypass it:

• The MCP launch site spreads mcp.environment into the child process without resolving remaining ${VAR} patterns
• External MCP discovery (Claude Code, Cursor, Copilot, Gemini configs) parses JSON directly via parseJsonc(), skipping the substitute() pipeline entirely

The fix:

• resolveEnvVars() safety net at the MCP launch site — catches all code paths regardless of how the config was loaded
• readJsonSafe() in discovery now uses ConfigPaths.parseText() for interpolation before parsing, with graceful fallback

14 new tests, all passing (importing from production source, not duplicated):

Covers all syntaxes: ${VAR}, ${VAR:-default}, {env:VAR}, $${VAR} escape, unset vars, plain passthrough, multiple vars in one value, bare $VAR not matched, and 3 discovery integration tests with .vscode/mcp.json and .cursor/mcp.json.

Also addressed cubic-dev-ai review: resolveEnvVars is now exported from src/mcp/index.ts and imported in the test file instead of duplicating the implementation.

Thanks @VJ-yadav . Can you please fix the CI-issue?

@anandgupta42 Rebased onto latest main (v0.5.20) and CI re-ran — same 7 failures, all pre-existing infra flakes unrelated to this PR:

• worker.test.ts:67 — TUI worker type assertion
• dbt-first-execution.test.ts — dispatcher call failures (4 tests)
• tracing-adversarial-2.test.ts:588 — fetch error
• oauth-callback.ts:188 — OAuth browser test
• ripgrep.ts:238 — file listing

None of our changed files (mcp/index.ts, mcp/discover.ts, test/mcp/env-var-interpolation.test.ts) are involved. Same failures showing on PR #649 and other open PRs in the batch. 7020 tests pass, only these 7 flaky ones fail.

THanks @VJ-yadav

Multi-Model Code Review — REQUEST CHANGES

Thanks for tackling #656 — the underlying bug is real and the two-layer approach is directionally right. Reviewed by a 9-participant panel (Claude + 8 external models: GPT 5.4, Gemini 3.1 Pro, Kimi K2.5, MiniMax M2.7, GLM-5, Qwen 3.6 Plus, DeepSeek V3, MiMo-V2-Pro). Consensus verdict: REQUEST CHANGES — 1 Critical, 4 Major, 5 Minor, 3 NIT.

The big one is a double-interpolation regression that breaks $${VAR} escaping end-to-end and opens a minor variable-chain injection vector. Details below.

Critical

1. Double interpolation — $${VAR} escape broken + variable-chain injection

Location: packages/opencode/src/mcp/discover.ts:117-130 + packages/opencode/src/mcp/index.ts:28-50 (call site around line 534)

discoverExternalMcp now runs configs through ConfigPaths.parseText → substitute() (Layer 1), and then at spawn time resolveEnvVars() runs again on the already-substituted mcp.environment (Layer 2). Two independent failures:

(a) $${VAR} escape no longer survives end-to-end. Layer 1 correctly turns $${VAR} into the literal ${VAR}. Layer 2 then sees that literal ${VAR} (negative lookbehind passes — there's no preceding $ anymore) and resolves it to the live env value. After this PR there is no syntax a user can write to pass a literal ${SOMETHING} string to an MCP server. Regression from pre-PR behavior for opencode.json configs.

(b) Variable-chain injection. If any env var in the shell has a value that looks like ${OTHER}, a config referencing that first var silently expands to the second one:

EVIL_VAR="${SECRET}"   # in shell (common in Bash PS1, GH Actions leftovers, etc.)
config: { environment: { X: "${EVIL_VAR}" } }
Layer 1: X = "${SECRET}"        (literal, as intended)
Layer 2: X = <actual SECRET value>   ← wrong

Gemini independently confirmed the bug with a running POC test.

Fix — pick one layer:

1. Preferred: scope Layer 1 to env blocks only (apply substitution inside addServersFromFile/transform, not in readJsonSafe over whole JSON text) and drop Layer 2 entirely. This also fixes Major chore(deps): Bump @gitlab/gitlab-ai-provider from 3.6.0 to 4.1.0 #5 below.
2. Alternative: track provenance on Config.Mcp entries and run resolveEnvVars only on those that bypassed ConfigPaths.parseText() (e.g., added via updateGlobal or at runtime).

Please gate the fix on a regression test that asserts $${VAR} survives to the child process as a literal ${VAR}, plus a test for the chain-injection vector.

Major

2. Regex/resolution logic duplicated from ConfigPaths.substitute() with silent divergence

Location: packages/opencode/src/mcp/index.ts:29-48 vs packages/opencode/src/config/paths.ts:104-131

ENV_VAR_PATTERN is a character-for-character copy of the regex inside substitute(). The bodies are near-identical but diverge in three ways:

The raw-vs-JSON-escape split is justified by the call sites, but there's no shared source of truth. The next regex fix will be applied to only one copy.

Fix: Extract a shared core in config/paths.ts:

export function resolveEnvVarPatterns(
  input: string,
  opts: { mode: "json-escaped" | "raw"; onUnresolved?: (name: string) => void }
): string

Both substitute() and resolveEnvVars() delegate to it — single regex, single resolution path.

3. Silent empty-string substitution on unresolved ${VAR}

Location: packages/opencode/src/mcp/index.ts:38-40

const envValue = process.env[dollarVar]
return envValue !== undefined && envValue !== "" ? envValue : (dollarDefault ?? "")

An unset ${GITLAB_TOKEN} with no default becomes "". The MCP server then launches with GITLAB_TOKEN="" and fails with an opaque auth error. No log, no warning, no telemetry. Makes diagnosing "why isn't my MCP server connecting?" needlessly painful.

Fix: Accept a serverName parameter and emit per unresolved reference:

log.warn("MCP env var unresolved, substituting empty string", { server: serverName, var: dollarVar })

4. Fallback catch {} in readJsonSafe swallows the error

Location: packages/opencode/src/mcp/discover.ts:124-127

} catch {
  log.debug("env-var interpolation failed for external MCP config, falling back to direct parse", { file: filePath })
}

ConfigPaths.parseText() can throw JsonError, InvalidError, or fail during substitution. All swallowed without the error message, and log.debug is invisible in normal operation.

Fix:

} catch (error) {
  log.warn("env-var interpolation failed for external MCP config, falling back to raw parse", {
    file: filePath,
    error: error instanceof Error ? error.message : String(error),
  })
}

5. Layer 1 substitution operates on whole JSON text, not just env fields

Location: packages/opencode/src/mcp/discover.ts:117-130

ConfigPaths.parseText runs substitute() over the full external-config JSON text — every ${VAR} anywhere in the file is resolved (command strings, URLs, args, server names), not just values inside env blocks. Foreign configs (Cursor, Copilot, Gemini) may use custom templating conventions that get clobbered.

Fix: Limit interpolation to env blocks inside addServersFromFile/transform rather than wholesale text substitution in readJsonSafe. Implementing this also cleanly eliminates Critical #1 without needing provenance tracking.

Minor

6. resolveEnvVars exported at top level instead of under MCP namespace

packages/opencode/src/mcp/index.ts:34 — every other symbol in the file is nested under export namespace MCP { ... }. Move inside the namespace and update the test import to MCP.resolveEnvVars(...).

7. mcp.headers not resolved — Authorization: Bearer ${TOKEN} still leaks through as literal

PR fixes mcp.environment but not mcp.headers. Remote MCP configs commonly put tokens in Authorization headers; the discovery sources you target (.vscode/mcp.json, .cursor/mcp.json) all support a headers field. A config with headers: { Authorization: "Bearer ${GITLAB_TOKEN}" } still hits the original bug.

Fix: Apply the resolver (single implementation per #2) to mcp.headers as well. Add a test.

8. {env:VAR} silently does not support :-default

packages/opencode/src/mcp/index.ts:31, 40-42 — the regex branch \{env:([^}]+)\} greedily captures :-default as part of the variable name. A user writing {env:VAR:-default} gets process.env["VAR:-default"], which never matches. The inline comment is also misleading about the supported syntax.

Fix: Either add explicit support (\{env:([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}) or add a test asserting the current non-support and correct the comment.

9. Missing integration tests for .copilot, .gemini, ~/.claude.json discovery sources

The integration tests only exercise .vscode/mcp.json and .cursor/mcp.json. If discovery for the other sources differs in how env blocks are located, the fix may silently not apply there.

Fix: Add tests for .copilot/mcp.json, .gemini/settings.json, and both the project-scoped and global ~/.claude.json MCP paths.

10. No test for readJsonSafe fallback path

No test covers the case where ConfigPaths.parseText() throws and execution falls through to raw parseJsonc. This is load-bearing for error resilience and the site of #4. Worth adding a test that triggers InvalidError inside parseText (e.g., a {file:missing} reference) and asserts the fallback still returns a parsed server entry.

NIT

11. Dynamic import("../config/paths") inside readJsonSafe runs per file

packages/opencode/src/mcp/discover.ts:121 — presumably avoiding a circular dep. If none exists, switch to a top-of-file static import. Otherwise hoist to module scope so it resolves once per process.

12. Test teardown process.env = { ...ORIGINAL_ENV } reassigns the object reference

packages/opencode/test/mcp/env-var-interpolation.test.ts:20, 97 — replacing the reference can desync from the native OS env backing store and leak keys into later tests. Prefer delete process.env[key] for each key set in beforeEach.

13. ENV_VAR_PATTERN at module scope with /g flag

packages/opencode/src/mcp/index.ts:31 — mutable lastIndex state, safe only with .replace(). Current single call-site is fine, but a future exec()/.test() caller would silently break. Document the contract or recreate the regex inside the function.

Positive observations

• Two-layer defense concept is architecturally sound. The bug report correctly identified multiple code paths; the instinct to cover all of them is right. The issue is the layers aren't isolated by provenance.
• $${VAR} literal escape is implemented and tested in isolation — many implementations forget the escape.
• Negative lookbehind (?<!\$) correctly prevents $$${VAR} overmatching at the regex level.
• Fall-through behavior on interpolation failure is conservative — the config still parses, even if without env resolution.
• altimate_change markers are correctly placed on all three modified files, satisfying Marker Guard CI.
• mcp.environment ? resolveEnvVars(...) : {} handles undefined correctly — the previous bare spread would have thrown on null.
• Test suite organization (unit direct + integration via discoverExternalMcp) is good structure.
• Tests import from the production module rather than re-implementing the helper — avoids stale-copy drift.

Missing tests to add

1. End-to-end $${VAR} preservation — load opencode.json with $${VAR} under mcp.environment, assert child process env receives the literal ${VAR}. Regression gate for Critical chore(deps): Bump minimatch from 10.0.3 to 10.2.3 in /packages/altimate-code #1.
2. Variable-chain injection — EVIL_VAR="${SECRET}", config references ${EVIL_VAR}, assert result is the literal ${SECRET}, not SECRET's value.
3. .copilot/mcp.json, .gemini/settings.json, ~/.claude.json project + global paths — see chore(deps-dev): Bump @types/yargs from 17.0.33 to 17.0.35 #9.
4. readJsonSafe fallback path — see chore(deps): Bump @parcel/watcher from 2.5.1 to 2.5.6 #10.
5. mcp.headers env resolution — see chore(deps-dev): Bump @babel/core from 7.28.4 to 7.29.0 #7.
6. Unresolved ${VAR} emits a warning — once Open-source readiness: engine bootstrap, branding, release infra #3 is implemented.
7. Env var value containing $ (non-template) — e.g., password p$ssword. Assert it survives unchanged.

Rejected findings (for transparency)

• "Dead code in readJsonSafe — fallback unreachable" was flagged CRITICAL by 2 models initially, both accepted the rejection at convergence. The return undefined lives inside the catch of the outer readText try/catch. On successful read, execution falls through to the new altimate_change block, which is fully reachable. Both branches work as intended.
• "Test file copy-pastes ENV_VAR_PATTERN and resolveEnvVars" — flagged as a concern, confirmed not applicable to PR head. Tests import from the production module.

Recommended minimum fix set before merge

1. Fix Critical chore(deps): Bump minimatch from 10.0.3 to 10.2.3 in /packages/altimate-code #1 (pick one resolution layer)
2. Address Majors chore(deps): Bump @modelcontextprotocol/sdk from 1.25.2 to 1.26.0 in /packages/altimate-code #2, Open-source readiness: engine bootstrap, branding, release infra #3, chore(deps): Bump @ai-sdk/xai from 2.0.51 to 3.0.60 #4
3. Add the Critical chore(deps): Bump minimatch from 10.0.3 to 10.2.3 in /packages/altimate-code #1 regression test and chain-injection test
4. Preferred direction: implement Major chore(deps): Bump @gitlab/gitlab-ai-provider from 3.6.0 to 4.1.0 #5 (scope Layer 1 to env blocks), which cleanly eliminates the double-pass without requiring provenance tracking

Minors #6–#10 can be follow-ups if you prefer, but #7 (headers) is a genuine gap in the stated fix scope for #656 and probably should land with the same PR.

Happy to re-review once changes are in.